ESO-494:Adds .md files for External-Secrets Operator#150
Open
siddhibhor-56 wants to merge 3 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds documentation for repository workflows, operator architecture, API contracts, integration patterns, performance conventions, security requirements, and testing practices. ChangesDocumentation guideline suite
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/testing-guidelines.md (1)
17-113: 💤 Low valueConsider adding blank lines around headings for Markdown compliance.
Multiple headings violate MD022 (blanks-around-headings). While this is a minor formatting issue, adding blank lines improves readability and passes linting. Given the "Chill" review preference, this is optional.
Example:
## Unit Tests (`pkg/`) ### Conventions + - Files live alongside source in the same package (e.g., `deployments.go` / `deployments_test.go`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/testing-guidelines.md` around lines 17 - 113, The Markdown file has multiple headings without surrounding blank lines (violating MD022); fix by inserting a single blank line before and after each heading token (e.g., the "### Conventions", "### Fake Client Pattern", "### Test Helpers", "### Error Assertion", "### Adding a New Unit Test", "## API Integration Tests (`test/apis/`)", "### Data-Driven Test Suites", "### envtest Setup", "### Running", "### Adding a New API Test", "## E2E Tests (`test/e2e/`)", "### Build Tag", "### Label-Based Platform Selection", "### E2E Structure", "### Test Data", "### E2E Test Utilities (`test/utils/`)", "### Adding a New E2E Test") so each heading is preceded and followed by exactly one blank line to satisfy lint MD022 and improve readability.CLAUDE.md (1)
9-51: ⚡ Quick winAdd blank lines around headings and code fences for Markdown compliance.
Multiple sections violate MD022 (blanks-around-headings) and MD031 (blanks-around-fences). While the content renders correctly in most viewers, proper spacing improves readability and passes linting.
📐 Example fix for one section
### Before Committing + ```bash make update && make verify
This regenerates all code, manifests, and bindata, then runs verification checks. CI will reject PRs with stale generated files.
Apply similar spacing to all other sections (Development Workflow, Testing, Dependency Management, Claude Code Preferences). </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CLAUDE.mdaround lines 9 - 51, Add a blank line before and after each
Markdown heading and before and after each fenced code block in CLAUDE.md (e.g.,
the "Before Committing", "Development Workflow", "Testing", "Dependency
Management", and "Claude Code Preferences" headings and theirbashfences)
so the file complies with MD022 and MD031; specifically, ensure there is an
empty line separating the heading from the previous paragraph and an empty line
between the fenced code opening/closing lines and surrounding text throughout
the file.</details> <!-- cr-comment:v1:fc5ea020d20ce7d1cb3e1afc --> </blockquote></details> <details> <summary>docs/api-contracts-guidelines.md (1)</summary><blockquote> `23-23`: _💤 Low value_ **Consider addressing markdown formatting for better consistency.** The markdownlint tool flagged several minor style issues: - Line 23: The code block should specify a language (e.g., ````go`) and be surrounded by blank lines - Several headings (lines 38, 43, 47, 54, 57, 82, 90, 95) should have blank lines before and after them These don't affect functionality but would improve markdown parsing consistency and potentially enable better syntax highlighting. Also applies to: 38-38, 43-43, 47-47, 54-54, 57-57, 82-82, 90-90, 95-95 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/api-contracts-guidelines.mdat line 23, Update the markdown for
docs/api-contracts-guidelines.md by adding a language specifier to the code
fence at the shown code block (e.g., changetogo) and ensure there is a
blank line before and after that fenced code block; also add a blank line before
and after each top-level heading referenced in the review (the headings at the
noted locations) so headings are separated from surrounding paragraphs and lists
for consistent markdownlint-compliant formatting.</details> <!-- cr-comment:v1:4182dd4ed530dcbd4d4b986e --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>docs/performance-guidelines.md (2)</summary><blockquote> `1-93`: _⚡ Quick win_ **Add blank lines below section headings for consistency.** Multiple level-3 headings (`###`) are missing blank lines below them before content begins (lines 5, 12, 19, 24, 38, 43, 51, 54, 59, 62, 65, 70, 77, 88, 91). Markdown best practice requires blank lines around headings for improved readability and consistency. <details> <summary>Example fix pattern</summary> ```diff ### Manager-Level Label-Filtered Cache + The operator uses a single manager-level cache... ``` Apply this pattern to all flagged headings. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance-guidelines.md` around lines 1 - 93, Add a single blank line after each level-3 Markdown heading that currently runs straight into content so headings follow the "blank line below headings" convention; specifically insert a blank line after the headings "### Manager-Level Label-Filtered Cache", "### Uncached Client Usage", "### Conditional CRD Cache Registration", "### Predicate Strategy by Resource Type", "### Map Function Convention", "### Error Classification and Requeue Strategy", "### Condition Update Optimization", "### CR Annotation Patching", "### UpdateWithRetry Pattern", "### Status Update Pattern", "### Bootstrap Resource Creation", "### Resettable sync.Once (`common.Now`)", "### HasObjectChanged Pattern", and "### Asset Decoding" in the document so each heading is followed by one empty line before its paragraph content. ``` </details> <!-- cr-comment:v1:e77c49c434d3ab410a8685d0 --> _Source: Linters/SAST tools_ --- `5-21`: _⚡ Quick win_ **Consider removing hard-coded line reference.** Line 8 references "line ~68" in `controller.go`. Hard-coded line numbers in documentation become stale as code evolves. Consider using a more resilient reference like "in `controllerManagedResources` constant" or "in the resource list near the cache builder initialization". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance-guidelines.md` around lines 5 - 21, The doc references a hard-coded line number ("line ~68" in controller.go); replace that brittle reference with a resilient identifier such as the constant or symbol name (e.g., controllerManagedResources) and/or a contextual location (e.g., "the resource list near NewCacheBuilder() / buildCacheObjectList() in pkg/controller/external_secrets/controller.go") so readers can find where to register new resources without relying on line numbers. ``` </details> <!-- cr-comment:v1:ebe16097cce0cd6e4abe1069 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@AGENTS.md:
- Around line 28-49: The fenced directory-listing block in AGENTS.md is missing
a language identifier; update the opening triple-backticks that precede the
directory list (the block containing lines like "api/v1alpha1/ CRD
type definitions...") to include a plaintext identifier (e.g.,text) so the code fence renders and lints correctly; ensure only the first backtick fence is changed and the closing fence remainsunchanged.In
@CLAUDE.md:
- Line 3: Replace the nonstandard annotation '
@AGENTS.md' in CLAUDE.md with a
proper Markdown link or an explanatory comment; specifically locate the literal
'@AGENTS.md' token and either change it to a Markdown link like
AGENTS.md or, if it is tool-specific, add a short inline comment
explaining the syntax and its purpose so readers know why it isn't standard
Markdown.In
@docs/security-guidelines.md:
- Around line 51-55: The markdown fenced code block showing the regex for
disallowedLabelMatcher (in install_external_secrets.go) needs proper formatting:
add a blank line before and after the fence and include a language identifier
(e.g., "regex") on the opening fence so the block renders correctly; update the
docs/security-guidelines.md block accordingly around the regex snippet for
disallowedLabelMatcher.
Nitpick comments:
In@CLAUDE.md:
- Around line 9-51: Add a blank line before and after each Markdown heading and
before and after each fenced code block in CLAUDE.md (e.g., the "Before
Committing", "Development Workflow", "Testing", "Dependency Management", and
"Claude Code Preferences" headings and theirbashfences) so the file
complies with MD022 and MD031; specifically, ensure there is an empty line
separating the heading from the previous paragraph and an empty line between the
fenced code opening/closing lines and surrounding text throughout the file.In
@docs/api-contracts-guidelines.md:
- Line 23: Update the markdown for docs/api-contracts-guidelines.md by adding a
language specifier to the code fence at the shown code block (e.g., changetogo) and ensure there is a blank line before and after that fenced code
block; also add a blank line before and after each top-level heading referenced
in the review (the headings at the noted locations) so headings are separated
from surrounding paragraphs and lists for consistent markdownlint-compliant
formatting.In
@docs/performance-guidelines.md:
- Around line 1-93: Add a single blank line after each level-3 Markdown heading
that currently runs straight into content so headings follow the "blank line
below headings" convention; specifically insert a blank line after the headings
"### Manager-Level Label-Filtered Cache", "### Uncached Client Usage", "###
Conditional CRD Cache Registration", "### Predicate Strategy by Resource Type",
"### Map Function Convention", "### Error Classification and Requeue Strategy",
"### Condition Update Optimization", "### CR Annotation Patching", "###
UpdateWithRetry Pattern", "### Status Update Pattern", "### Bootstrap Resource
Creation", "### Resettable sync.Once (common.Now)", "### HasObjectChanged
Pattern", and "### Asset Decoding" in the document so each heading is followed
by one empty line before its paragraph content.- Around line 5-21: The doc references a hard-coded line number ("line ~68" in
controller.go); replace that brittle reference with a resilient identifier such
as the constant or symbol name (e.g., controllerManagedResources) and/or a
contextual location (e.g., "the resource list near NewCacheBuilder() /
buildCacheObjectList() in pkg/controller/external_secrets/controller.go") so
readers can find where to register new resources without relying on line
numbers.In
@docs/testing-guidelines.md:
- Around line 17-113: The Markdown file has multiple headings without
surrounding blank lines (violating MD022); fix by inserting a single blank line
before and after each heading token (e.g., the "### Conventions", "### Fake
Client Pattern", "### Test Helpers", "### Error Assertion", "### Adding a New
Unit Test", "## API Integration Tests (test/apis/)", "### Data-Driven Test
Suites", "### envtest Setup", "### Running", "### Adding a New API Test", "##
E2E Tests (test/e2e/)", "### Build Tag", "### Label-Based Platform Selection",
"### E2E Structure", "### Test Data", "### E2E Test Utilities (test/utils/)",
"### Adding a New E2E Test") so each heading is preceded and followed by exactly
one blank line to satisfy lint MD022 and improve readability.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: openshift/coderabbit/.coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `088f4b9d-3b92-48d9-a894-246e50996f77` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e9969c3b3dbb343ccdc9e781d9feffd549b74d71 and 13849aa8957958b3770a70ca98889d060d8ee63a. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `AGENTS.md` * `CLAUDE.md` * `docs/api-contracts-guidelines.md` * `docs/error-handling-guidelines.md` * `docs/integration-guidelines.md` * `docs/performance-guidelines.md` * `docs/security-guidelines.md` * `docs/testing-guidelines.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| @@ -0,0 +1,51 @@ | |||
| # CLAUDE.md | |||
|
|
|||
| @AGENTS.md | |||
There was a problem hiding this comment.
Clarify or remove the @AGENTS.md annotation.
The @AGENTS.md syntax is not standard Markdown. If this is intended as a reference or link, use a proper Markdown link: [AGENTS.md](AGENTS.md). If it's meant for a specific tool or extension, add a comment explaining the syntax.
🔗 Proposed fix
# CLAUDE.md
-@AGENTS.md
+See [AGENTS.md](AGENTS.md) for comprehensive architectural details.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @AGENTS.md | |
| # CLAUDE.md | |
| See [AGENTS.md](AGENTS.md) for comprehensive architectural details. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 3, Replace the nonstandard annotation '`@AGENTS.md`' in
CLAUDE.md with a proper Markdown link or an explanatory comment; specifically
locate the literal '`@AGENTS.md`' token and either change it to a Markdown link
like [AGENTS.md](AGENTS.md) or, if it is tool-specific, add a short inline
comment explaining the syntax and its purpose so readers know why it isn't
standard Markdown.
Contributor
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
5-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNormalize the heading and fence spacing.
markdownlintstill flags MD022/MD031 in the command sections. Add blank lines around the headings and fenced blocks so the doc passes lint cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 5 - 52, The command section markdown in CLAUDE.md still violates markdownlint MD022/MD031; update the Build and Test Commands and related headings so they are surrounded by blank lines, and ensure each fenced command block has the proper blank line spacing before and after it. Keep the existing content but normalize the spacing around the headings and code fences so the document passes lint cleanly.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 95-99: The reconciliation-flow markdown block in ARCHITECTURE.md
is missing a language tag on the second fenced block, which trips markdownlint.
Update the relevant fenced section so it opens with text, matching the existing
code fence style used in the document, and keep the change localized to the
reconciliation-flow description.
- Around line 13-43: Add a text language tag to the fenced ASCII diagram so
markdownlint no longer reports MD040; update the diagram fence in
ARCHITECTURE.md to open as text while keeping the existing diagram content
unchanged. Locate the fenced block containing the ExternalSecretsConfig
architecture diagram and adjust only the opening fence.
In `@docs/error-handling-guidelines.md`:
- Around line 91-94: The `CreateDefaultESMResource` retry flow is treating
`TooManyRequests` as a terminal stop condition, which prevents backoff and
retry. Update the `shouldRetryOnError` predicate used by `retry.OnError` in
`externalsecretsmanager.go` to remove `TooManyRequests` from the stop list,
while keeping the other non-retryable errors intact, so 429 responses continue
through retry logic instead of returning success early.
In `@docs/integration-guidelines.md`:
- Around line 80-84: Clarify the label rules in the integration guidelines: the
current “managed resources carry app: external-secrets” requirement conflicts
with the disallowed “app” prefix in the blocklist. Update the wording around the
managed label set and the selector/cache filter contract in this section so it
clearly states whether app is required, exempted, or should be replaced by
another label key, keeping the guidance consistent with the validation rules.
In `@docs/testing-guidelines.md`:
- Around line 3-131: The Markdown in testing-guidelines.md needs spacing
normalization to satisfy markdownlint (MD022/MD031). Update the document so
headings in the Test Architecture, Unit Tests, API Integration Tests, E2E Tests,
Coverage, and CI Notes sections have proper blank lines before/after them, and
ensure any tables or fenced-style blocks are separated by blank lines where
needed. Focus on the formatting around the section headings and the main content
blocks so the file renders cleanly and passes lint.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 5-52: The command section markdown in CLAUDE.md still violates
markdownlint MD022/MD031; update the Build and Test Commands and related
headings so they are surrounded by blank lines, and ensure each fenced command
block has the proper blank line spacing before and after it. Keep the existing
content but normalize the spacing around the headings and code fences so the
document passes lint cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0edcaf7-3570-40f7-a01d-6a9e32e1ff93
📒 Files selected for processing (10)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
Comment on lines
+91
to
+94
| ### `retry.OnError` with Custom Predicate | ||
|
|
||
| The ESM default resource creation (`pkg/controller/external_secrets_manager/externalsecretsmanager.go`) uses `retry.OnError` with a custom `shouldRetryOnError` predicate that stops retrying on `AlreadyExists`, `Conflict`, `Invalid`, `BadRequest`, `Unauthorized`, `Forbidden`, and `TooManyRequests`. | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== doc snippet ==\n'
sed -n '80,105p' docs/error-handling-guidelines.md
printf '\n== controller references ==\n'
rg -n "shouldRetryOnError|TooManyRequests|retry.OnError" pkg/controller/external_secrets_manager -S
printf '\n== file outline ==\n'
ast-grep outline pkg/controller/external_secrets_manager/externalsecretsmanager.go --view expandedRepository: openshift/external-secrets-operator
Length of output: 2302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== externalsecretsmanager.go ==\n'
cat -n pkg/controller/external_secrets_manager/externalsecretsmanager.go
printf '\n== call sites ==\n'
rg -n "CreateDefaultESMResource\(" -S .Repository: openshift/external-secrets-operator
Length of output: 2616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== setup_manager.go ==\n'
cat -n pkg/operator/setup_manager.goRepository: openshift/external-secrets-operator
Length of output: 2568
Remove TooManyRequests from the stop list. CreateDefaultESMResource treats stop-list errors as success (return nil inside the retry.OnError callback), so a 429 currently skips the create entirely instead of backing off and retrying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/error-handling-guidelines.md` around lines 91 - 94, The
`CreateDefaultESMResource` retry flow is treating `TooManyRequests` as a
terminal stop condition, which prevents backoff and retry. Update the
`shouldRetryOnError` predicate used by `retry.OnError` in
`externalsecretsmanager.go` to remove `TooManyRequests` from the stop list,
while keeping the other non-retryable errors intact, so 429 responses continue
through retry logic instead of returning success early.
Comment on lines
+3
to
+131
| ## Test Architecture | ||
|
|
||
| This repo has three test tiers. The repo uses `go.work` with modules at `.`, `./test`, `./cmd/external-secrets-operator`, `./tools`. Unit tests live in the root `.` module; API and E2E tests share the `./test` module: | ||
|
|
||
| | Tier | Location | Make Target | Framework | Cluster Required | | ||
| |------|----------|-------------|-----------|-----------------| | ||
| | Unit | `pkg/**/*_test.go` | `make test-unit` | stdlib `testing` + table-driven | No | | ||
| | API Integration | `test/apis/` | `make test-apis` | Ginkgo/Gomega + envtest | No (envtest) | | ||
| | E2E | `test/e2e/` | `make test-e2e` | Ginkgo/Gomega + live cluster | Yes | | ||
|
|
||
| `make test` runs `manifests`, `generate`, `fmt`, `vet`, then both `test-apis` and `test-unit` (no cluster needed). It also builds envtest binaries as a prerequisite. | ||
|
|
||
| ## Unit Tests (`pkg/`) | ||
|
|
||
| ### Conventions | ||
| - Files live alongside source in the same package (e.g., `deployments.go` / `deployments_test.go`). | ||
| - Use stdlib `testing` package exclusively; no Ginkgo. | ||
| - Follow table-driven test pattern: define a `tests` slice of structs with `name`, `preReq`, `wantErr`, and optional validation funcs, then iterate with `t.Run`. | ||
| - No build tags; unit tests compile under normal `go test`. | ||
|
|
||
| ### Fake Client Pattern | ||
| - Use `pkg/controller/client/fakes/FakeCtrlClient` (generated by counterfeiter) to mock the controller-runtime client. | ||
| - Set up behavior per test via `mock.ExistsCalls(...)`, `mock.CreateCalls(...)`, `mock.UpdateWithRetryCalls(...)`, etc. | ||
| - Assign mock to `r.CtrlClient = mock` on the test reconciler. | ||
|
|
||
| ### Test Helpers | ||
| - `pkg/controller/external_secrets/test_utils.go` provides `testReconciler(t)`, `testDeployment(name)`, `testService(name)`, `testResourceMetadata(esc)`, and typed decode helpers (`testClusterRole`, `testSecret`, etc.). | ||
| - `pkg/controller/commontest/utils.go` provides `TestExternalSecretsConfig()`, `TestExternalSecretsManager()`, `ErrTestClient`, and standard test constants (`TestExternalSecretsImageName`, `TestBitwardenImageName`, `TestExternalSecretsNamespace`). | ||
|
|
||
| ### Error Assertion | ||
| - Compare exact error strings: `if err == nil || err.Error() != tt.wantErr`. | ||
| - Use `t.Setenv()` for environment variables needed by the code under test (e.g., `RELATED_IMAGE_EXTERNAL_SECRETS`). | ||
|
|
||
| ### Adding a New Unit Test | ||
| 1. Create or edit `*_test.go` next to the source file, same package. | ||
| 2. Define a table-driven test with `name`, mock setup (`preReq`), optional input mutation (`updateExternalSecretsConfig`), and `wantErr` string. | ||
| 3. Use `testReconciler(t)` for the reconciler, `fakes.FakeCtrlClient{}` for the client. | ||
| 4. Assert with `t.Errorf`; avoid external assertion libraries in unit tests. | ||
|
|
||
| ## API Integration Tests (`test/apis/`) | ||
|
|
||
| ### Data-Driven Test Suites | ||
| - Tests are defined in YAML files at `api/v1alpha1/tests/<crd-name>/<name>.testsuite.yaml`. | ||
| - Each file specifies `crdName`, `tests.onCreate` (create + validate or expect error), and `tests.onUpdate` (create, update, validate or expect error). | ||
| - The Go code in `test/apis/generator.go` auto-generates Ginkgo specs from these YAML files using `DescribeTable`. | ||
|
|
||
| ### envtest Setup | ||
| - Suite file: `test/apis/suite_test.go`. | ||
| - Bootstraps `envtest.Environment` with CRDs from `config/crd/bases/`. | ||
| - Uses Kubernetes API version >= 1.25 (required for CEL validation). | ||
| - Uses `komega` for object comparison with `IgnoreAutogeneratedMetadata`. | ||
|
|
||
| ### Running | ||
| - `make test-apis` invokes `hack/test-apis.sh`, which runs Ginkgo with `--randomize-all --randomize-suites --keep-going --timeout=30m`. | ||
| - In CI (`OPENSHIFT_CI=true`), JUnit and coverage artifacts are written to `ARTIFACT_DIR`. | ||
|
|
||
| ### Adding a New API Test | ||
| 1. Create or edit a `.testsuite.yaml` file under `api/v1alpha1/tests/<crd-name>/`. | ||
| 2. Define `onCreate` entries with `initial` YAML (input) and either `expected` YAML (success) or `expectedError` (failure substring). | ||
| 3. Define `onUpdate` entries with `initial`, `updated`, `expected`/`expectedError`/`expectedStatusError`. Optionally use `initialCRDPatches` for ratcheting tests. | ||
| 4. The generator picks them up automatically; no Go code changes needed. | ||
|
|
||
| ## E2E Tests (`test/e2e/`) | ||
|
|
||
| ### Build Tag | ||
| - All files in `test/e2e/` and `test/utils/` require `//go:build e2e`. The Makefile passes `-tags e2e` via `go test -C test`. | ||
|
|
||
| ### Label-Based Platform Selection | ||
| Tests are filtered by Ginkgo labels. The default filter is `"Platform: isSubsetOf {AWS}"`. | ||
|
|
||
| | Label | Scope | Required Secrets | | ||
| |-------|-------|-----------------| | ||
| | `Platform:AWS` | AWS SecretStore + ExternalSecret + PushSecret | `aws-creds` in `kube-system` | | ||
| | `Platform:Generic` | Platform-independent operand tests | None | | ||
| | `CrossPlatform:GCP-AWS` | GCP cluster using AWS Secrets Manager | `aws-creds` in `kube-system` | | ||
| | `Provider:Bitwarden` | Bitwarden ESO provider via ClusterSecretStore | `bitwarden-creds` in `external-secrets-operator` | | ||
| | `API:Bitwarden` | Direct HTTP tests to bitwarden-sdk-server | `bitwarden-creds` in `external-secrets-operator` | | ||
| | `Suite:Bitwarden` | Full Bitwarden suite | `bitwarden-creds` in `external-secrets-operator` | | ||
| | `Proxy:HTTP` | Proxy-related tests | Proxy configuration | | ||
| | `Migration` | Migration/upgrade tests | None | | ||
| | `PostUpgradeCheck` | Post-upgrade verification | None | | ||
|
|
||
| Note: some E2E test contexts (e.g., Environment Variables, Deployment Revision History, Annotations, Network Policies, Trusted CA Bundle) have **no platform label** and only run when the label filter is empty (`E2E_GINKGO_LABEL_FILTER=""`). | ||
|
|
||
| Run a specific suite: `make test-e2e E2E_GINKGO_LABEL_FILTER="Provider:Bitwarden"`. | ||
| Run all: `make test-e2e E2E_GINKGO_LABEL_FILTER=""`. | ||
|
|
||
| ### E2E Structure | ||
| - Suite setup (`e2e_suite_test.go`): `BeforeSuite` initializes K8s clients (`kubernetes.Clientset`, `dynamic.DynamicClient`, `controller-runtime client.Client`), sets timeout with 5-minute cleanup buffer, configures JSON/JUnit reports. `AfterSuite` runs global `CleanupESOOperandAndRelated`. | ||
| - `BeforeAll` (in `e2e_test.go`): creates test namespace (prefix `external-secrets-e2e-test-`), creates/verifies `ExternalSecretsConfig` CR, waits for operator/operand pods. | ||
| - `BeforeEach`: verifies operand pods are ready. | ||
| - `AfterEach`: on failure, dumps artifacts via `utils.DumpE2EArtifacts()` to `ARTIFACT_DIR` or `_output/`. | ||
| - `AfterAll` (in `e2e_test.go`): deletes test namespace and cluster CR. | ||
|
|
||
| ### Test Data | ||
| - YAML fixtures embedded via `//go:embed testdata/*` in `e2e_test.go`. | ||
| - Pattern substitution in YAML via `utils.ReplacePatternInAsset("${PATTERN}", "value")`. | ||
| - Fixtures live in `test/e2e/testdata/`. | ||
|
|
||
| ### E2E Test Utilities (`test/utils/`) | ||
| Key helpers (all require `e2e` build tag): | ||
| - `DynamicResourceLoader`: create/delete K8s resources from YAML files or `Unstructured` objects. | ||
| - `VerifyPodsReadyByPrefix(ctx, clientset, namespace, prefixes)`: poll until pods with given name prefixes are Running + Ready. | ||
| - `WaitForESOResourceReady(ctx, dynamicClient, gvr, namespace, name, timeout)`: poll until ESO resource has `Ready=True` condition. | ||
| - `WaitForExternalSecretsConfigReady(ctx, dynamicClient, name, timeout)`: wait for both `Ready=True` and `Degraded=False`. | ||
| - `CleanupESOOperandAndRelated(ctx, cfg)`: best-effort cleanup of operand CRs, webhooks, RBAC, namespace. | ||
| - `GetRandomString(n)`: random alphanumeric string for unique resource names. | ||
|
|
||
| ### Adding a New E2E Test | ||
| 1. Add `//go:build e2e` at the top of any new file. | ||
| 2. Place test in `test/e2e/` package; use existing `suiteClientset`, `suiteDynamicClient`, `suiteRuntimeClient`. | ||
| 3. Apply a Ginkgo `Label(...)` to the `Context` or `Describe` for platform filtering. | ||
| 4. Use `Ordered` for tests with shared state dependencies. | ||
| 5. Create test namespace with `GenerateName: testNamespacePrefix`; clean up in `AfterAll`. | ||
| 6. Use `Eventually(...).Should(Succeed())` for async assertions with polling. | ||
| 7. Use `defer loader.DeleteFromFile(...)` for resource cleanup. | ||
| 8. Add YAML fixtures to `test/e2e/testdata/`. | ||
|
|
||
| ## Coverage | ||
|
|
||
| - `make test-unit` writes `cover.out` (Go coverage profile) to the repo root. | ||
| - E2E coverage uses a special instrumented image: `make docker-build-coverage` builds from `images/ci/Dockerfile.coverage`. `make e2e-coverage-collect` gathers profiles from the running operator and optionally uploads to Codecov. | ||
|
|
||
| ## CI Notes | ||
|
|
||
| - `GOFLAGS` is cleared for test targets (line: `fmt vet test test-unit test-e2e run update-vendor update-dep: GOFLAGS=`) to avoid conflicts with `go.work`. | ||
| - envtest uses Kubernetes `1.32.0` binaries (`ENVTEST_K8S_VERSION`). | ||
| - E2E timeout defaults to `1h` (`E2E_TIMEOUT`); Ginkgo suite timeout is set to `E2E_TIMEOUT - 5min`. | ||
| - `ARTIFACT_DIR` controls where CI artifacts (JUnit XML, JSON reports, failure dumps) are written. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Normalize the Markdown block spacing.
This file still trips MD022/MD031 across most sections. Add blank lines around the headings and fenced blocks so it passes markdownlint.
🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Ensure spelling is correct
Context: ...lient{}for the client. 4. Assert witht.Errorf`; avoid external assertion libraries in u...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 23-23: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 28-28: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 55-55: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 67-67: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 70-70: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 90-90: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 97-97: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 102-102: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 111-111: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/testing-guidelines.md` around lines 3 - 131, The Markdown in
testing-guidelines.md needs spacing normalization to satisfy markdownlint
(MD022/MD031). Update the document so headings in the Test Architecture, Unit
Tests, API Integration Tests, E2E Tests, Coverage, and CI Notes sections have
proper blank lines before/after them, and ensure any tables or fenced-style
blocks are separated by blank lines where needed. Focus on the formatting around
the section headings and the main content blocks so the file renders cleanly and
passes lint.
Source: Linters/SAST tools
siddhibhor-56
changed the title
bharath-b-rh
left a comment
bharath-b-rh
left a comment
Contributor
There was a problem hiding this comment.
Still going through, just a few suggestions for now.
|
|
||
| This operator does NOT use kubebuilder-generated admission webhooks for its own CRDs. CEL-based CRD validation handles all admission validation. The webhook code in `pkg/controller/external_secrets/` manages the upstream external-secrets operand's `ValidatingWebhookConfiguration` resources (not the operator's own API validation). | ||
|
|
||
| ## Adding New API Fields Checklist |
Contributor
There was a problem hiding this comment.
Shouldn't we add a section on Godoc too?
The Godoc on any field in the API should be sufficiently explained such that an end user understands the purpose, interactions, limitations, and default behavior.
| 3. Call `StatusUpdate` (status subresource update). | ||
| 4. Wrap in `retry.RetryOnConflict(retry.DefaultRetry, ...)`. | ||
|
|
||
| ## Defaults |
Contributor
There was a problem hiding this comment.
Lets add below detail to avoid defining defaults in CRD going forward.
Excerpt from OpenShift API conventions
With configuration APIs, we typically default fields within the controller and not within the API. This means that the platform has the ability to make changes to the defaults over time.
| @@ -0,0 +1,157 @@ | |||
| # API Contracts Guidelines | |||
Contributor
There was a problem hiding this comment.
We should also add a rule not define any functions in API package.
| @@ -0,0 +1,131 @@ | |||
| # Testing Guidelines | |||
Contributor
There was a problem hiding this comment.
This section will need an update when we merge the e2e restructuring PR. Mainly around the labeling of test cases.
anandkuma77
added a commit
to anandkuma77/external-secrets-operator
that referenced
this pull request
Jul 1, 2026
Extends PR openshift#150 with focused Track 1 Contextualization additions: Added ADRs (Architecture Decision Records): - docs/decisions/001-bindata-over-helm.md - Why bindata pattern - docs/decisions/002-three-controller-design.md - Why three controllers - docs/decisions/003-cel-validation-only.md - Why CEL validation - docs/decisions/004-singleton-cr-pattern.md - Why singleton CRs - docs/decisions/README.md - ADR index Added Core Documentation: - GLOSSARY.md - Domain-specific terminology (bindata, ESC/ESM, etc.) - docs/CONTEXT_MAP.md - Documentation navigation map These additions provide: - Prescriptive context explaining architectural "why" (ADRs) - Essential terminology for AI agents and developers (GLOSSARY) - Clear documentation organization (CONTEXT_MAP) All documentation follows Track 1 principles: - Grounded in source code (file paths, line numbers) - Separates prescriptive from descriptive context - Designed for both human and AI consumption Ready for Track 1 Phase 2 context registration in Cyborg. Total additions: 7 files focusing on core architectural context.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-contracts-guidelines.md (1)
5-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the
init()exception.Line 5 bans all functions in
api/v1alpha1/, but Line 11 requiresSchemeBuilder.Register()inside aninit()in the same package. Please either exempt this minimal registration function or reword the rule; as written, the guidance is self-contradictory.✍️ Suggested wording
-- **No functions in the API package.** The `api/v1alpha1/` package must contain only type definitions, constants, markers, and generated code (deepcopy). +- **No business-logic functions in the API package.** The `api/v1alpha1/` package must contain only type definitions, constants, markers, generated code (deepcopy), and the minimal `init()` registration needed for `SchemeBuilder`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-contracts-guidelines.md` around lines 5 - 11, The API package guidance is internally inconsistent because it bans all functions in api/v1alpha1/ while also requiring an init() there for SchemeBuilder.Register(). Reword the “No functions in the API package” rule to explicitly exempt the minimal init() used only for scheme registration, and keep the exception aligned with the SchemeBuilder.Register() requirement in the types file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/api-contracts-guidelines.md`:
- Around line 5-11: The API package guidance is internally inconsistent because
it bans all functions in api/v1alpha1/ while also requiring an init() there for
SchemeBuilder.Register(). Reword the “No functions in the API package” rule to
explicitly exempt the minimal init() used only for scheme registration, and keep
the exception aligned with the SchemeBuilder.Register() requirement in the types
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ade6c0d5-07c1-4764-9c39-2c39e609f561
📒 Files selected for processing (5)
AGENTS.mdARCHITECTURE.mddocs/api-contracts-guidelines.mddocs/integration-guidelines.mddocs/security-guidelines.md
✅ Files skipped from review due to trivial changes (4)
- docs/integration-guidelines.md
- ARCHITECTURE.md
- docs/security-guidelines.md
- AGENTS.md
|
@siddhibhor-56: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references ESO-494 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit